e2e test for storage class diskformat verification for vsphere cloud provider #41239

divyenpatel · 2017-02-10T06:52:25Z

What this PR does / why we need it:
This PR adds a new e2e test for vsphere cloud provider.
Test is to verify diskformat specified in storage-class is being honored while volume creation.

Steps:

Create StorageClass with diskformat set to valid type (supported options are eagerzeroedthick, zeroedthick and thin)
Create PVC which uses the StorageClass created in step 1.
Wait for PV to be provisioned.
Wait for PVC's status to become Bound
Create POD using PVC on specific node.
Wait for Disk to be attached to the node.
Get node VM's devices and find PV's Volume Disk.
Get Backing Info of the Volume Disk and obtain Property of VirtualDiskFlatVer2BackingInfo - EagerlyScrub and ThinProvisioned
Based on the value of EagerlyScrub and ThinProvisioned, verify if diskformat is correct.
Delete POD and Wait for Volume Disk to be detached from the Node.
Delete PVC, PV and Storage Class

Which issue this PR fixes *
fixes #

Special notes for your reviewer:
Test is executed against v1.6.0-alpha.1
Test is failing on v1.4.8

Release Note

NONE

@kerneltime @BaluDontu @abrarshivani please review this PR

k8s-ci-robot · 2017-02-10T06:52:26Z

Hi @divyenpatel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-reviewable · 2017-02-10T06:53:31Z

This change is

kerneltime · 2017-02-10T18:14:19Z

@k8s-bot ok to test

divyenpatel · 2017-02-10T18:20:04Z

@jeffvance can you also review?

jeffvance · 2017-02-10T18:39:20Z

@divyenpatel probably need to add
"```release-note
NONE
```"
tag to this pr.

jeffvance · 2017-02-10T18:45:21Z

pkg/cloudprovider/providers/vsphere/vsphere_util.go

+	if err != nil {
+		return nil, err
+	}
+	vs := VSphere{


why not vs := &VSphere{ ... ?
Then remove & below? There may be a good reason for not doing this but it is a fairly common pattern.

fixed. followed common pattern.

jeffvance · 2017-02-10T18:47:08Z

test/e2e/vsphere_utils.go

@@ -178,3 +179,89 @@ func verifyContentOfVSpherePV(client clientset.Interface, pvc *v1.PersistentVolu
 	runInPodWithVolume(client, pvc.Namespace, pvc.Name, "grep '"+expectedContent+"' /mnt/test/data")
 	framework.Logf("Sucessfully verified content of the volume")


"Successfully..."

jeffvance · 2017-02-10T18:50:57Z

test/e2e/vsphere_utils.go

+	scAnnotation := make(map[string]string)
+	scAnnotation["volume.beta.kubernetes.io/storage-class"] = storageclass.Name
+
+	claim := v1.PersistentVolumeClaim{


same question about why not using:
claim := &v1.PersistentVolumeClaim{

fixed. followed common pattern.

jeffvance · 2017-02-10T18:53:26Z

test/e2e/vsphere_volume_diskformat.go

+	11. Delete PVC, PV and Storage Class
+*/
+
+var _ = framework.KubeDescribe("Volume Disk Format", func() {


consider adding [Volume] and [vsphere] tags

done.
framework.KubeDescribe("Volume Disk Format [Feature:Volumes] [vsphere]", func() {
Also added vsphere tag in vsphere_volume_placement.go

grep -R "\[Feature:Volumes\]" test | wc -l 3

grep -R "\[Volume\]" test |wc -l 82

so, for now, [Volume] (singular) is far more common.

ok, will use Volume then

jeffvance · 2017-02-10T19:55:47Z

test/e2e/vsphere_volume_diskformat.go

+		client.CoreV1().PersistentVolumeClaims(namespace).Delete(pvclaimSpec.Name, nil)
+	}()
+
+	By("Waiting for calim to be in bould phase")


"claim", "bound"

fixed. Will turn on spell checker my IDE next time.

jeffvance · 2017-02-10T19:59:00Z

test/e2e/vsphere_volume_diskformat.go

+		PV is required to be attached to the Node. so that using govmomi API we can grab Disk's Backing Info
+		to check EagerlyScrub and ThinProvisioned property
+	*/
+	By("Creating POD to attach PV to the node")


nit: "POD" is not an acronym like "PV" is. Usually lowercase "pod" is displayed in messages.

jeffvance · 2017-02-10T20:00:20Z

test/e2e/vsphere_volume_diskformat.go

+	By("Creating POD to attach PV to the node")
+	// Create POD to attach Volume to Node
+	podSpec := getVSpherePodSpecWithClaim(pvclaim.Name, nodeKeyValueLabel, "while true ; do sleep 2 ; done")
+	pod, err := client.CoreV1().Pods(namespace).Create(podSpec)


test for err missing.

added assert check for all err

jeffvance · 2017-02-10T20:04:07Z

test/e2e/vsphere_volume_diskformat.go

+	f := find.NewFinder(govMoMiClient.Client, true)
+	ctx, _ := context.WithCancel(context.Background())
+	vm, err := f.VirtualMachine(ctx, os.Getenv("VSPHERE_WORKING_DIR")+nodeName)
+	vmDevices, err := vm.Device(ctx)


is an err check needed here?

jeffvance · 2017-02-11T03:28:33Z

test/e2e/vsphere_volume_diskformat.go

 	11. Delete PVC, PV and Storage Class
 */

-var _ = framework.KubeDescribe("Volume Disk Format", func() {
+var _ = framework.KubeDescribe("Volume Disk Format [Feature:Volume] [vsphere]", func() {


I don't fully understand the difference between "[Feature:xyz]" vs "[xyz]". This doc describes "Feature:" as:
"If a test has non-default requirements to run or targets some non-core functionality, and thus should not be run as part of the standard suite. "
So, my choice would be to simply use [Volume] as the tag, as done in ~82 other tests.

I thought it would be easier to filter vsphere related tests with adding [vsphere] tag.
but if this is not the convention, let’s remove this.

jeffvance · 2017-02-11T03:31:30Z

test/e2e/vsphere_volume_diskformat.go

@@ -109,17 +109,21 @@ func invokeTest(client clientset.Interface, namespace string, nodeName string, n
 	By("Creating Storage Class With DiskFormat")
 	storageClassSpec := getVSphereStorageClassSpec("thinsc", scParameters)
 	storageclass, err := client.StorageV1beta1().StorageClasses().Create(storageClassSpec)
+	Expect(err).NotTo(HaveOccurred())
+
 	defer client.StorageV1beta1().StorageClasses().Delete(storageclass.Name, nil)
 	Expect(err).NotTo(HaveOccurred())


redundant with L112. I may have given a poor review comment previously?

removed redundant check. Thanks for detailed review.

jeffvance · 2017-02-11T03:33:49Z

test/e2e/vsphere_volume_placement.go

@@ -28,7 +28,7 @@ import (
 	"k8s.io/kubernetes/test/e2e/framework"
 )

-var _ = framework.KubeDescribe("Volume Placement [Feature:Volume]", func() {
+var _ = framework.KubeDescribe("Volume Placement [Feature:Volume] [vsphere]", func() {


same "Feature:" comment as above.

kerneltime · 2017-02-13T17:49:45Z

/approve

spxtr · 2017-02-13T18:11:14Z

/approve

divyenpatel · 2017-02-14T17:23:36Z

@jeffvance addressed all review comments.

jeffvance · 2017-02-15T02:45:03Z

test/e2e/vsphere_volume_diskformat.go

+	11. Delete PVC, PV and Storage Class
+*/
+
+var _ = framework.KubeDescribe("Volume Disk Format [Feature:Volume]", func() {


There's been some back and forth here. IMO, this line should be:
var _ = framework.KubeDescribe("Volume Disk Format [Volumes]", func() {...
In a previous comment I've shown about 25x more occurences in test/ of my suggestion vs what you have above. But I may be misunderstanding how [Feature:xyz]` works, or vsphere may have special needs here.

Fixed. There is no special need in vpshere. Just followed same pattern as in volumes.go

jeffvance · 2017-02-15T02:48:04Z

@divyenpatel One comment above. Overall, looks good to me; however I am not an assignee so I cannot add LGTM.

adding govmomi dep to test/e2e/BUILD adding golang.org/x/net/context to e2e deps addressed review comments addressed 2nd round of review comments addressed review comments regarding KubeDescribe tagging

k8s-github-robot · 2017-02-15T05:10:45Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: divyenpatel, kerneltime, spxtr

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/vsphere/OWNERS~~ [kerneltime]
~~test/OWNERS~~ [spxtr]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

divyenpatel · 2017-02-15T05:26:32Z

@deads2k can you review and approve this change?

jeffvance · 2017-02-15T18:10:43Z

@k8s-bot verify test this

divyenpatel · 2017-02-15T20:17:36Z

@jeffvance Jenkins verification passed.

jeffvance · 2017-02-15T20:40:15Z

/lgtm

k8s-github-robot · 2017-02-16T04:05:08Z

Automatic merge from submit-queue (batch tested with PRs 37137, 41506, 41239, 41511, 37953)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2017

k8s-github-robot assigned deads2k Feb 10, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 10, 2017

jeffvance reviewed Feb 10, 2017

View reviewed changes

k8s-github-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Feb 10, 2017

jeffvance reviewed Feb 10, 2017

View reviewed changes

jeffvance reviewed Feb 11, 2017

View reviewed changes

divyenpatel force-pushed the e2eTestsUpdate-v2 branch from eeadec8 to 6ce2dfb Compare February 11, 2017 05:43

divyenpatel mentioned this pull request Feb 11, 2017

Add missing e2e tests vmware-archive/kubernetes-archived#64

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2017

jeffvance reviewed Feb 15, 2017

View reviewed changes

adding e2e test for storage class diskformat verification

cdb48fb

adding govmomi dep to test/e2e/BUILD adding golang.org/x/net/context to e2e deps addressed review comments addressed 2nd round of review comments addressed review comments regarding KubeDescribe tagging

divyenpatel force-pushed the e2eTestsUpdate-v2 branch from 6ce2dfb to cdb48fb Compare February 15, 2017 05:10

deads2k assigned jeffvance and unassigned deads2k Feb 15, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2017

k8s-github-robot merged commit 6823803 into kubernetes:master Feb 16, 2017

divyenpatel deleted the e2eTestsUpdate-v2 branch September 5, 2017 23:11

		@@ -178,3 +179,89 @@ func verifyContentOfVSpherePV(client clientset.Interface, pvc *v1.PersistentVolu
		runInPodWithVolume(client, pvc.Namespace, pvc.Name, "grep '"+expectedContent+"' /mnt/test/data")
		framework.Logf("Sucessfully verified content of the volume")

e2e test for storage class diskformat verification for vsphere cloud provider #41239

e2e test for storage class diskformat verification for vsphere cloud provider #41239

Conversation

divyenpatel commented Feb 10, 2017 • edited

k8s-ci-robot commented Feb 10, 2017

k8s-reviewable commented Feb 10, 2017

kerneltime commented Feb 10, 2017

divyenpatel commented Feb 10, 2017

jeffvance commented Feb 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Feb 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Feb 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Feb 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Feb 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Feb 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerneltime commented Feb 13, 2017

spxtr commented Feb 13, 2017

divyenpatel commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance commented Feb 15, 2017

k8s-github-robot commented Feb 15, 2017

divyenpatel commented Feb 15, 2017

jeffvance commented Feb 15, 2017

divyenpatel commented Feb 15, 2017

jeffvance commented Feb 15, 2017

k8s-github-robot commented Feb 16, 2017

divyenpatel commented Feb 10, 2017 •

edited

jeffvance commented Feb 10, 2017 •

edited

jeffvance Feb 10, 2017 •

edited

jeffvance Feb 10, 2017 •

edited

jeffvance Feb 10, 2017 •

edited

jeffvance Feb 11, 2017 •

edited

jeffvance Feb 11, 2017 •

edited